Skip to content

refactor(mercury): remove sha1_smol dependency#739

Merged
genedna merged 3 commits into
gitmono-dev:mainfrom
el-ev:remove_sha1_smol
Dec 12, 2024
Merged

refactor(mercury): remove sha1_smol dependency#739
genedna merged 3 commits into
gitmono-dev:mainfrom
el-ev:remove_sha1_smol

Conversation

@el-ev

@el-ev el-ev commented Dec 12, 2024

Copy link
Copy Markdown
Contributor

There are two different SHA1 implementations (i.e. sha1 and sha1_smol) and a custom SHA1 Wrapper exist simultaneously in the codebase, which doesn't make sense.
Since sha1 is more widely used across the project, it should be kept in favor of sha1_smol.

@vercel

vercel Bot commented Dec 12, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mega ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2024 1:38pm

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (2)

mercury/src/hash.rs:66

  • The error message should be more descriptive. Consider changing it to: 'Invalid input length: expected a 40-character SHA-1 hash string.'
return Err("The length of the string is not 40".to_string());

mercury/src/hash.rs:101

  • Ensure that the behavior of sha1::Sha1::digest is covered by tests.
let h = sha1::Sha1::digest(data);

@Hou-Xiaoxuan Hou-Xiaoxuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@genedna
genedna added this pull request to the merge queue Dec 12, 2024
Merged via the queue into gitmono-dev:main with commit e49a8b6 Dec 12, 2024
@el-ev
el-ev deleted the remove_sha1_smol branch December 13, 2024 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants